Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switch formatting to ruff format #7531

Closed
wants to merge 3 commits into from
Closed

Switch formatting to ruff format #7531

wants to merge 3 commits into from

Conversation

akx
Copy link
Contributor

@akx akx commented Nov 7, 2023 •
edited
Loading

For consideration, following #6966.

I had to move some # fmt: off / # fmt: on comments a bit, because ruff format doesn't read them within expressions. Otherwise, code style remains the same.

With no caches (--no-cache set for Ruff too), this makes lint-fix about 10 times faster on my machine:

$ env BLACK_CACHE_DIR=/dev/null hyperfine --warmup 5 'git checkout main; make lint-fix' 'git checkout ruff-format-no-cache; make lint-fix'
Benchmark 1: git checkout main; make lint-fix
  Time (mean ± σ):      1.410 s ±  0.025 s    [User: 9.183 s, System: 0.477 s]
  Range (min … max):    1.385 s …  1.459 s    10 runs

Benchmark 2: git checkout ruff-format-no-cache; make lint-fix
  Time (mean ± σ):     134.1 ms ±   2.4 ms    [User: 356.4 ms, System: 81.6 ms]
  Range (min … max):   130.2 ms … 140.1 ms    21 runs

Summary
  git checkout ruff-format-no-cache; make lint-fix ran
   10.52 ± 0.27 times faster than git checkout main; make lint-fix

@hugovk
Copy link
Member

hugovk commented Nov 7, 2023

git checkout ruff-format-no-cache - where is this branch?

Thanks, though as mentioned in #6966, I'd prefer to keep Black rather than the Ruff formatter.

A few milliseconds doesn't make much difference: normal human use will be with caches and mostly-formatted code, and on the CI it won't matter compared to the jobs that take half an hour.

@hugovk hugovk marked this pull request as draft November 7, 2023 14:51
@akx
Copy link
Contributor Author

akx commented Nov 7, 2023

git checkout ruff-format-no-cache - where is this branch?

Pushed nowhere – did open astral-sh/ruff#8538 (which apparently got merged already, nice) to make it unnecessary.

The diff is

diff --git a/Makefile b/Makefile
index 7e0419f20..aacd9a6c8 100644
--- a/Makefile
+++ b/Makefile
@@ -118,5 +118,5 @@ lint:
 .PHONY: lint-fix
 lint-fix:
        python3 -c "import ruff" > /dev/null 2>&1 || python3 -m pip install ruff
-       python3 -m ruff --fix .
-       python3 -m ruff format .
+       python3 -m ruff --no-cache --fix .
+       python3 -m ruff format --no-cache .

Thanks, though as mentioned in #6966, I'd prefer to keep Black rather than the Ruff formatter.

Sure, I recall seeing that mention. Is there a technical reason?

A few milliseconds doesn't make much difference: normal human use will be with caches and mostly-formatted code, and on the CI it won't matter compared to the jobs that take half an hour.

Well, IMO every saved cycle (and watt of electricity) does matter.

@hugovk
Copy link
Member

hugovk commented Nov 7, 2023

One reason is it's so new and it'll take some time to stabilise. Black is working well, there's no strong reason to switch.

If we want to save a few cycles or watts of electricity, there are much bigger gains to be found elsewhere rather than a few milliseconds here.

@radarhere radarhere closed this Nov 14, 2023
@akx akx mentioned this pull request Aug 6, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Do Not Merge Needs Rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@akx @hugovk @radarhere